Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

child_process: validate strings in exec and spawn #56148

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

child_process: validate strings in exec and spawn #56148

wants to merge 1 commit into from
+107 −12

Conversation

puskin94
Copy link
Contributor

@puskin94 puskin94 commented Dec 5, 2024 •
edited
Loading

I went through the exec, execFile, spawn, execSync, execFileSync and spawnSync functions in child_process and edited all the functions to properly validate their string parameters

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Dec 5, 2024
@puskin94 puskin94 force-pushed the child-process-docs-and-validation branch from 358bd79 to 40a0a9f Compare December 5, 2024 17:12
@codecov Codecov
Copy link

codecov bot commented Dec 5, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.53%. Comparing base (3c2da4b) to head (a3b5b1a).
Report is 48 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #56148      +/-   ##
==========================================
+ Coverage   87.99%   88.53%   +0.54%     
==========================================
  Files         656      657       +1     
  Lines      188999   189882     +883     
  Branches    35981    36461     +480     
==========================================
+ Hits       166301   168120    +1819     
+ Misses      15865    14970     -895     
+ Partials     6833     6792      -41
Files with missing lines Coverage Δ
lib/child_process.js 97.73% <100.00%> (+0.30%) ⬆️

... and 107 files with indirect coverage changes

aduh95
aduh95 reviewed Dec 8, 2024
View reviewed changes
lib/child_process.js Outdated
// Set the shell, switches, and commands.
if (process.platform === 'win32') {
if (typeof options.shell === 'string')
file = options.shell;
command = options.shell;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change makes the code more confusing, IMO it makes more sense to keep file for the shell executable and command for the actual command.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing it back now. Do you still agree on changing the documentation in the code to reflect the terminology actually used in the documentation in .md files?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify? I'm not sure I follow

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what I mean:

https://nodejs.org/api/child_process.html#child_processspawncommand-args-options

in the documentation, the first parameter is called "command" (which makes sense), while in the code it is called "file" , which is confusing.

The initial commit I did which I just reverted, also changed the name of the parameters in the code to match the proper name used in the documentation.

Copy link
Contributor

@aduh95 aduh95 Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure "command" is much better than "file" tbh. According to https://stackoverflow.com/a/2051031, ISO C11 refers to it as the "program name". I would call it "child process argv0".
In any case, let's not do that in this PR, it can be its own PR.

aduh95
aduh95 reviewed Dec 8, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please remove all the unrelated changes? It makes the PR hard to review. Please only include changes that are necessary to make the added test pass, and all the other changes should be made in a separate PR.

@puskin94 puskin94 force-pushed the child-process-docs-and-validation branch from 40a0a9f to 18ddc47 Compare December 9, 2024 07:55
@puskin94 puskin94 force-pushed the child-process-docs-and-validation branch from 18ddc47 to 6d02bc8 Compare December 9, 2024 11:21
@puskin94 puskin94 force-pushed the child-process-docs-and-validation branch from 6d02bc8 to 0a84382 Compare December 9, 2024 13:43
@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 10, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 10, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63969/

@puskin94 puskin94 changed the title child_process: improve docs and validate strings in exec and spawn child_process: validate strings in exec and spawn Dec 10, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63992/

aduh95
aduh95 requested changes Dec 11, 2024
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the added test pass on main, meaning either the change in lib/ is only a refactor, or we are not adding sufficient coverage to avoid regression.

@puskin94
Copy link
Contributor Author

It looks like the added test pass on main, meaning either the change in lib/ is only a refactor, or we are not adding sufficient coverage to avoid regression.

it was mainly refactoring because, for example, when calling execFile, the validation was not done immediately but down the road when, at the very end, the method itself was calling the spawn function.
Tests are passing in main because the validation in spawn is already present; my change is mainly about doing it as soon as possible to reject as soon as possible in order to save some cycles and having a better stack trace

@aduh95
Copy link
Contributor

aduh95 commented Dec 11, 2024

in order to save some cycles and having a better stack trace

I don't think it's the right approach, we should optimize for the happy path, where there are no error thrown. IIUC, with this change we would be checking twice if the arguments are valid, so in order to save some cycles we should not land this.

@puskin94 puskin94 force-pushed the child-process-docs-and-validation branch from 0a84382 to a3b5b1a Compare December 11, 2024 11:19
@puskin94
Copy link
Contributor Author

in order to save some cycles and having a better stack trace

I don't think it's the right approach, we should optimize for the happy path, where there are no error thrown. IIUC, with this change we would be checking twice if the arguments are valid, so in order to save some cycles we should not land this.

gotcha. I went in that direction because I noticed that was the case already. With the latest push all the validation is done down the line and only once

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aduh95 aduh95 aduh95 requested changes

@ZYSzys ZYSzys ZYSzys approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@puskin94 @nodejs-github-bot @aduh95 @ZYSzys